Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

S3900: Basic rule implementation #6969

Merged
merged 16 commits into from
Mar 27, 2023
Merged

S3900: Basic rule implementation #6969

merged 16 commits into from
Mar 27, 2023

Conversation

zsolt-kolbay-sonarsource
Copy link
Contributor

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource commented Mar 21, 2023 •
edited
Loading

Part of #6793
Task 4

This PR works without the implementation of ShouldExecute(), so some of test cases have FPs, due to the visibility of the method not being checked.
Re-assignment of the method parameters will be taken care of in a separate PR, as well as the re-organization/cleanup of the test cases.

Tim-Pohlmann
Tim-Pohlmann requested changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough to check for method visibility in ShouldExecute? I feel the rule's implementation should not rely on whether or not the rule will be executed.
It might work in this specific instance, but it will cause issues when we update ShouldExecute to be broader.

...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...st/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp8.cs Outdated Show resolved Hide resolved
antonioaversa
antonioaversa requested changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Releasing a first bunch of comments, so that you can go through while I check the test files.

...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...r.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
antonioaversa
antonioaversa approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@antonioaversa antonioaversa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just an additional comment about updating IT JSONs.

Tim-Pohlmann
Tim-Pohlmann approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@Tim-Pohlmann Tim-Pohlmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor nitpicks only.

...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...t/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp11.cs Outdated
Comment on lines 11 to 18
public void Compliant(object[] o)
{
if (o is [not null, not null])
{
o.ToString(); // Compliant
o.ToString(); // Noncompliant - FP
o[1].ToString(); // Compliant
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to split this into two different tests. Otherwise, the engine will assume not null for o in line 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

o is not null regardless of whether I split it up or not due to the pattern matching. Could you show exactly what that would look like?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the engine doesn't think it's NotNull, otherwise, it wouldn't raise. I assume the next line might be a FP as well. Right now line 16 is just not really testing anything.

if (o is [not null, not null])
{
    o.ToString();       // Noncompliant - FP
}
if (o is [not null, not null])
{
    o[1].ToString();       // Noncompliant - FP
}

...st/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.CSharp9.cs Show resolved Hide resolved
pavel-mikula-sonarsource
pavel-mikula-sonarsource requested changes Mar 23, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor polishing

...r.UnitTest/TestCases/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
...SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/PublicMethodArgumentsShouldBeCheckedForNull.cs Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

pavel-mikula-sonarsource
pavel-mikula-sonarsource approved these changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@pavel-mikula-sonarsource pavel-mikula-sonarsource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pavel-mikula-sonarsource pavel-mikula-sonarsource merged commit 7e9dccf into feature/SE Mar 27, 2023
@pavel-mikula-sonarsource pavel-mikula-sonarsource deleted the Zsolt/S3900-basic-rule-implementation branch March 27, 2023 10:31
pavel-mikula-sonarsource pushed a commit that referenced this pull request Mar 28, 2023
@zsolt-kolbay-sonarsource @pavel-mikula-sonarsource
S3900: Basic rule implementation (#6969)
ecef0fd
pavel-mikula-sonarsource pushed a commit that referenced this pull request Mar 31, 2023
@zsolt-kolbay-sonarsource @pavel-mikula-sonarsource
S3900: Basic rule implementation (#6969)
16f60fc
pavel-mikula-sonarsource pushed a commit that referenced this pull request Apr 4, 2023
@zsolt-kolbay-sonarsource @pavel-mikula-sonarsource
S3900: Basic rule implementation (#6969)
17e0116
@antonioaversa antonioaversa mentioned this pull request Apr 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tim-Pohlmann Tim-Pohlmann Tim-Pohlmann approved these changes

@antonioaversa antonioaversa antonioaversa approved these changes

@pavel-mikula-sonarsource pavel-mikula-sonarsource pavel-mikula-sonarsource approved these changes

Assignees

@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zsolt-kolbay-sonarsource @antonioaversa @Tim-Pohlmann @pavel-mikula-sonarsource